Repository navigation
style: use the optional chains typescript-eslint 8.71 asks for - #3797
armando-navarro wants to merge 1 commit into
Conversation
The Angular 22 upgrade needs @typescript-eslint 8.71 for TypeScript 6, and that release reports prefer-optional-chain on these two checks, so lint fails once it lands. Both rewrites keep behavior. An undefined provided list still falls through to getApp(). config.name is always a non-empty string, so a missing app still never matches it.
armando-navarro
added
comp: compat
tyler-reitz
left a comment
tyler-reitz
left a comment
There was a problem hiding this comment.
Both rewrites check out. Holding the approval until 21.0.x is cut, since approval is the only gate here and would make this mergeable early. Ping me once the branch exists.
One correction to the reasoning, which does not change the fix. prefer-optional-chain is already enabled as error in 8.46's stylistic-type-checked, which eslint.config.js:28 pulls in, so the rule has been live all along. I linted both forms under both versions: 8.46.0 reports nothing on either, 8.71.1 reports exactly these two and nothing on the new lines. So the rule's detection broadened between the two versions rather than the rule arriving with 8.71.
On behavior, I ran the old and new forms side by side: five inputs for provided, and fourteen find combinations crossing both config names with arrays holding null, undefined, matching and non-matching apps. No divergences. Worth recording why the second one is safe: the two forms do differ when config.name is nullish, where the old form returns the matching object and the new one returns the null element, which sends existingApp || initializeApp(...) down different branches. It cannot happen because name falls back to '[DEFAULT]' and config.name = config.name || name two lines up, exactly as you say. That assignment is now load-bearing for the rewrite.
|
@tyler-reitz Thanks for linting both versions. You're right that the rule was already on in 8.46. The PR only says 8.71 reports these two lines, which matches what you found, so I've left the description as it is. This is a draft PR, so an approval doesn't make it mergeable until I pull it out of draft state. If it looks good, could you approve it now? It stays a draft until the |
tyler-reitz
left a comment
tyler-reitz
left a comment
There was a problem hiding this comment.
Approving while it is still a draft, per the 21.0.x hold in the description.
I ran both rewrites rather than reading them: a small node harness replicating the name resolution and both predicates over 7 nameOrConfig inputs and 5 app values reports no behavioral difference, and config.name is a non-empty string in every case, including {name: ''}. So an undefined app never matches, as you said.
Both branches are behind main and will need a rebase before merging.
Angular 22 removed ComponentFactoryResolver, which screen tracking used to read a routed component's selector. reflectComponentType(component)?.selector replaces it in both screen-tracking services, and ɵscreenViewEvent loses its resolver parameter. That replacement and the new screen-tracking spec come from #3741. Angular 22 requires TypeScript 6, which defaults strict on, drops downlevelIteration, deprecates baseUrl and moduleResolution node, and reports unresolvable side-effect imports. The build script now compiles under strict. The library keeps strict off for a separate migration. baseUrl goes, with tsconfig-paths 4 resolving the path mappings for the node specs. src/test.ts goes too: the Karma builder injects its own entry file and never ran it. It also carries the two prefer-optional-chain rewrites from #3797, which @typescript-eslint 8.71 requires. Both keep behavior. BREAKING CHANGE: @angular/fire 22 requires Angular 22, TypeScript 6.0 and Node 22.22.3, 24.15.0 or 26. Angular 21 applications stay on @angular/fire 21. Fixes #3696 Refs #3737 Co-authored-by: Ethan Sarif-Kattan <ethaneus99@gmail.com>
|
This commit landed on |
Caution
Do not merge until the
21.0.xbranch is cutThis is part of the Angular 22 upgrade and ships in 22.0.0-rc.0. Merge it after
21.0.xis cut frommain.Refs #3696
This rewrites two checks as optional chains, so lint keeps passing once the Angular 22 upgrade brings in a newer
@typescript-eslint.Changes
src/app/app.module.ts:provided && provided.length === 1becomesprovided?.length === 1.src/compat/firebase.app.module.ts:app && app.name === config.namebecomesapp?.name === config.name.Both rewrites keep behavior. An undefined
providedstill falls through togetApp(), and sinceconfig.nameis set to a non-empty string two lines above the check, a missingappstill never matches it.Why
@typescript-eslint8.46, the version in today's lockfile, accepts only TypeScript below 6.0, so the Angular 22 upgrade moves it to 8.71.@typescript-eslint/prefer-optional-chainon these two lines, sonpm run lintfails.Verification
With
@typescript-eslint8.71.1 on the Angular 22 upgrade, eslint reports exactly these two errors on the old lines and none on the new ones.There are no new tests, since the existing specs already run both lines through
provideFirebaseAppandAngularFireModule.initializeApp.